feat(security): warn once when enableSecureText is disabled + document XSS trade-off#480
Merged
Merged
Conversation
…t XSS trade-off enableSecureText defaults to false, so option label/value are written to the DOM as raw HTML. Consumers rendering untrusted data without opting in are exposed to XSS, with no signal that it is happening. Keep the default off (changing it would break intentional-HTML consumers and impose per-option escaping cost on large datasets), but make the trade-off discoverable: - One-time (per page) console.warn when options render while enableSecureText is disabled. O(1) static-flag guard; never scans option content, so there is no cost on large datasets. - properties.md: prominent security callout + expanded enableSecureText row. No behavioural/default change beyond the warning. Verified: npm run validate (tsc/eslint/stylelint) clean; npm run build succeeds; new spec secure-text-warning.cy.ts passes; existing security/perf/lifecycle specs (16 tests) still pass.
Adds a per-instance `showSecureTextWarning` boolean (default `true`) so a consumer who has consciously accepted the enableSecureText trade-off (trusted / developer-controlled option data, or intentional HTML) can silence the one-time console warning without enabling escaping. - New prop wired through the standard plumbing: dataProps registration, setDefaultProps default (true), setProps assignment, and the data-show-secure-text-warning attribute mapping. - warnIfSecureTextDisabled() now also returns early when the prop is false. Kept as a dedicated, clearly-named option rather than overloading the internal secureTextWarningShown lifecycle flag. - Documented in properties.md (new row + callout note) and the virtualSelectOptions typedef. Verified: npm run validate clean; npm run build succeeds; new test case asserts showSecureTextWarning:false suppresses the warning; full suite 222 tests pass (secure-text-warning 3, security specs 4, examples 215) - no regressions.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make the XSS trade-off of leaving enableSecureText disabled (the default) more discoverable by adding a one-time console warning and prominent documentation, while preserving current default behavior for performance reasons on large datasets.
Changes:
- Add a per-page, one-time
console.warnwhen rendering options withenableSecureText: false, with a per-instance opt-out viashowSecureTextWarning. - Document the XSS/performance trade-off prominently in
docs/properties.md, including the newshowSecureTextWarningproperty. - Add Cypress coverage verifying the warning fires once, and is suppressed when
enableSecureText: trueorshowSecureTextWarning: false.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/virtual-select.types.js | Extends the public options typedef with showSecureTextWarning. |
| src/virtual-select.js | Implements the one-time warning flag + new prop plumbing. |
| docs/properties.md | Adds a security callout and documents the new warning behavior/option. |
| cypress/e2e/secure-text-warning.cy.ts | Adds E2E coverage for the one-time warning behavior. |
| docs/assets/virtual-select.js | Generated docs bundle update reflecting new behavior. |
| dist/virtual-select.js | Generated distribution bundle update reflecting new behavior. |
| dist/virtual-select.min.js | Generated minified distribution bundle update reflecting new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… message & docs - warnIfSecureTextDisabled(): drop the empty-options guard so the once-per-page warning fires on configuration at construction. Previously a select initialised with no options (then populated later via setOptions/server search) never warned even though its option text renders as raw HTML. (Copilot: Medium) - warning message: broaden to cover label, value, description and customData used in markup (not just label/value). Trailing comma kept intentionally - airbnb-base comma-dangle requires it and the bundle is transpiled via @babel/preset-env to a browserslist that excludes IE11, so the IE11 parsing concern does not apply here. (Copilot: High - message reworded; IE11/trailing-comma part rejected) - docs/properties.md: stop claiming enableSecureText escapes 'customData fields'. It escapes the built-in fields (label, value, description); customData is stored as-is except where the library interpolates it, and labelRenderer/selectedLabelRenderer output is not sanitized. (Copilot: High x2) - test: add coverage asserting the warning fires when constructed with empty options. Bundles regenerated; npm run validate + build pass; secure-text-warning spec 4/4 green.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…nchor) Copilot review (Medium): the `?id=enablesecuretext` fragment does not resolve - docsify generates anchors from headings, but enableSecureText is a table row, not a heading (the repo's working ?id= links all target real headings). The security callout is at the top of properties.md, so the bare #/properties link lands the reader on it. Trailing comma kept intentionally (airbnb comma-dangle requires it; IE11 excluded from browserslist; bundle is transpiled), so the comma-removal part of the suggestion was not applied. Bundles regenerated; validate + build pass; secure-text-warning spec 4/4.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
enableSecureTextdefaults tofalse, so option text (label,value,description, and anycustomDatainterpolated into markup such as the optionaria-label) is written to the DOM as raw HTML. Consumers rendering untrusted data without opting in are exposed to XSS, and there is no signal that this is happening.Changing the default to
truewould be disruptive (breaks intentional-HTML consumers) and would impose per-option escaping cost on large datasets (10k–100k+ records) where it is often unnecessary. So this PR keeps the default off but makes the trade-off impossible to miss.Change
enableSecureTextis disabled, log a single (per page)console.warnpointing to the property docs. It fires on the configuration alone at construction — it is O(1) and never scans option content, so there is no cost on large datasets, and it is not missed when options are loaded later (e.g. viasetOptionsor server search). Tracked by a staticsecureTextWarningShownflag.properties.mdplus an expandedenableSecureTextrow. The docs are explicit about scope:enableSecureTextescapes the built-in fields (label, value, description);customDatais stored as-is except where the library itself interpolates it, and HTML returned bylabelRenderer/selectedLabelRendereris not sanitized.No behavioural/default change beyond the warning.
Opt-out:
showSecureTextWarningNew per-instance boolean option
showSecureTextWarning(defaulttrue). Set it tofalseto suppress the one-time warning once you have consciously accepted the trade-off (trusted/developer-controlled option data, or intentional HTML) — without paying for escaping viaenableSecureText.It is a dedicated, clearly-named option rather than an overload of the internal
secureTextWarningShownlifecycle flag, and is wired through the standard plumbing (dataProps,setDefaultProps,setProps,data-show-secure-text-warningattribute mapping) plus thevirtualSelectOptionstypedef andproperties.md.Verification
npm run validate(tsc + eslint + stylelint): cleannpm run build: succeeds;dist/docsbundles rebuiltcypress/e2e/secure-text-warning.cy.ts— 4 passing: warns exactly once across two instances; does not warn whenenableSecureText: true; does not warn whenshowSecureTextWarning: false; warns even when constructed with empty options.examplessuite.